Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move FormControl styles to PVC #1638

Merged
merged 17 commits into from
Dec 2, 2022
Merged

Move FormControl styles to PVC #1638

merged 17 commits into from
Dec 2, 2022

Conversation

simurai
Copy link
Contributor

@simurai simurai commented Nov 25, 2022

Description

This is part of #1342 and adds the FormControl styles from PCSS. There should be no visual changes.

Integration

Does this change require any updates to code in production?

Yes, the following lines can be removed on dotcom:

- @import '@primer/css/forms/FormControl.scss';

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews
  • Visual regression test

Not sure why the Primer::Beta::AutoComplete placeholder text is lighter in the snapshots. Locally they seem to look the same:

Before After
Screen Shot 2022-11-28 at 15 53 11 Screen Shot 2022-11-28 at 15 53 18

@changeset-bot
Copy link

changeset-bot bot commented Nov 25, 2022

🦋 Changeset detected

Latest commit: 1727564

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/view-components Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the css Pull requests that update CSS code label Nov 25, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Nov 25, 2022

⚠️ Visual differences found

Our visual comparison tests found UI differences. Please review the differences by viewing the files changed tab to ensure that the changes were intentional.

Review visual differences

@simurai simurai temporarily deployed to review-pr-1638 November 25, 2022 08:33 Inactive
@simurai simurai temporarily deployed to github-pages November 25, 2022 08:37 Inactive
@simurai simurai temporarily deployed to review-pr-1638 November 25, 2022 09:02 Inactive
@@ -15,6 +15,7 @@ module.exports = {
features: {
'nesting-rules': true,
'focus-visible-pseudo-class': false,
'logical-properties-and-values': false,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to disable logical-properties-and-values to make properties like padding-inline-start work.

When set to true, the output adds [dir="ltr"] + [dir="rtl"] to the selector, I assume as a fallback. But we currently don't use these attributes in Lookbook/dotcom.

@simurai simurai temporarily deployed to review-pr-1638 November 25, 2022 09:41 Inactive
@simurai simurai temporarily deployed to github-pages November 25, 2022 09:45 Inactive
@simurai simurai temporarily deployed to review-pr-1638 November 28, 2022 03:20 Inactive
@simurai simurai temporarily deployed to review-pr-1638 November 28, 2022 05:16 Inactive
@simurai simurai temporarily deployed to github-pages November 28, 2022 05:21 Inactive
@simurai simurai temporarily deployed to review-pr-1638 November 28, 2022 06:40 Inactive
@simurai simurai temporarily deployed to github-pages November 28, 2022 06:44 Inactive
@simurai simurai marked this pull request as ready for review November 28, 2022 06:57
@simurai simurai requested a review from a team as a code owner November 28, 2022 06:57
@simurai simurai requested review from a team, tobiasahlin and camertron November 28, 2022 06:57
Comment on lines +43 to +50
".FormControl-inlineValidation p",
".FormControl-select",
".FormControl-textarea",
".FormControl-input",
".FormControl-input-wrap",
".FormControl-select-wrap",
".FormControl-checkbox-wrap",
".FormControl-radio-wrap"
Copy link
Contributor Author

@simurai simurai Nov 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently all .FormControl styles are part of Primer::Alpha::TextField and located at app/components/primer/alpha/text_field.pcss. But the .FormControl styles are not just for text fields and also include styles for select, radio, checkbox etc. but it might not be easy to split up?

We could also move .FormControl to a place where we have styles that get shared across multiple components? Similar to app/lib/primer/css/ that currently get used for utilities.

@simurai simurai merged commit 5c42127 into main Dec 2, 2022
@simurai simurai deleted the forms-pcss branch December 2, 2022 02:37
@primer-css primer-css mentioned this pull request Dec 2, 2022
keithamus pushed a commit that referenced this pull request Dec 2, 2022
Co-authored-by: simurai <simurai@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css Pull requests that update CSS code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants